Introduce mapping transformer to allow transform mappings during index create/update or index template create/update #17635
Conversation
bzhangam
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
cwperks,
dblock,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami,
VachaShah,
bugmakerrrrrr,
jainankitk and
linuxpi
as code owners
github-actions
bot
added
Cluster Manager
enhancement
bzhangam
force-pushed
the
introduceMappingTransformer
branch
from
37b530b to
7d47df7
Compare
|
❌ Gradle check result for 7d47df7: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
bzhangam
force-pushed
the
introduceMappingTransformer
branch
2 times, most recently
from
6f9c569 to
dc11eee
Compare
|
❌ Gradle check result for dc11eee: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
...src/main/java/org/opensearch/action/admin/indices/mapping/put/TransportPutMappingAction.java
Outdated
Show resolved
Hide resolved
...va/org/opensearch/action/admin/indices/template/put/TransportPutComponentTemplateAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/index/mapper/MappingTransformer.java
Outdated
Show resolved
Hide resolved
bzhangam
force-pushed
the
introduceMappingTransformer
branch
from
dc11eee to
c35c386
Compare
|
❌ Gradle check result for c35c386: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
bzhangam
force-pushed
the
introduceMappingTransformer
branch
from
c35c386 to
2406d5b
Compare
bzhangam
force-pushed
the
introduceMappingTransformer
branch
from
2406d5b to
3ac7c23
Compare
github-project-automation
bot
moved this from ✅ Done
to 🏗 In progress
in Cluster Manager Project Board
|
❌ Gradle check result for b8204e8: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
bzhangam
force-pushed
the
introduceMappingTransformer
branch
2 times, most recently
from
6016e32 to
0103403
Compare
|
❌ Gradle check result for 0103403: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
server/src/main/java/org/opensearch/action/admin/indices/create/TransportCreateIndexAction.java
Show resolved
Hide resolved
server/src/main/java/org/opensearch/index/mapper/MappingTransformer.java
Show resolved
Hide resolved
| builder.map(mapping); | ||
| listener.onResponse(builder.toString()); | ||
| } catch (IOException e) { | ||
| listener.onFailure(new RuntimeException("Failed to parse the transformed mapping [" + mapping + "]", e)); |
There was a problem hiding this comment.
I understand this is a catch all for the transformation, but ideally we should have bailed out where the root cause of the transformation is (to help with debugging), because we have already validated user mapping by this point.
There was a problem hiding this comment.
I think this is not a catch all. It's more for the case that after the transformation it's possible some transformers corrupt the mapping and make it not a valid format. In that case we cannot parse the mapping back to a json string. The tricky part is that we don't know which transformer corrupt the mapping if there are multiple transformers because we don't want to parse the mapping in each transformer. Do you want to suggest a better error message?
There was a problem hiding this comment.
Yeah I was worried there would be no way to debug later. One option is to validate the parsing after each transformation. Do you know whats the worst case performance impact of parsing for each applyNext?. Its a trade off performance vs debuggable.
Possibly we can exposing a cluster setting which operators can tune and turn it off by default.
| @@ -511,7 +515,17 @@ private <Request extends ClusterManagerNodeRequest<Request>, Response extends Ac | |||
| masterNodeAction, | |||
| request, | |||
| clusterState, | |||
| new PlainActionFuture<>() | |||
| new ActionListener<Response>() { | |||
There was a problem hiding this comment.
Curious what prompted this change?
There was a problem hiding this comment.
The PlainActionFuture will not throw the exception on failure but there is a unit test that wants to verify the exception thrown during index creation. Before my change the exception will be threw since in the TransportCreateIndexAction we don't wrap the code to create the index. But now it's wrapped by a listener.
bzhangam
force-pushed
the
introduceMappingTransformer
branch
from
0103403 to
599b03a
Compare
…x create/update or index template create/update. Signed-off-by: Bo Zhang <[email protected]>
bzhangam
force-pushed
the
introduceMappingTransformer
branch
from
599b03a to
ceabd64
Compare
|
❕ Gradle check result for ceabd64: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17635 +/- ##
============================================
+ Coverage 72.44% 72.57% +0.13%
- Complexity 66801 66894 +93
============================================
Files 5455 5457 +2
Lines 309183 309281 +98
Branches 44987 44991 +4
============================================
+ Hits 223981 224471 +490
+ Misses 66873 66434 -439
- Partials 18329 18376 +47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Bo Zhang <[email protected]>
|
❕ Gradle check result for 077b13c: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Description
In this PR we introduce the mapping transformer to the core to allow transforming the mapping during index create/update and index template create/update. We want to do this because we have a use case in neural search plugin where we want to auto generate the semantic fields(e.g. knn_vector or rank_feature) in the mappings based on the model id defined in the semantic field(A new field type introduced by neural search plugin). In this way we can simplify the neural search set up.
Potentially in future this feature can be leveraged by other plugins to auto transform the mapping based on the certain config defined by users.
Related Issues
Resolves #[17500] - Proposal to introduce the mapping transformer into core.
#[803] - Proposal to support semantic field in neural search.
#[1211] - HLD of the semantic field.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.